SearchKit - Allow subsearch of different types - #34853
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
Directive can load any viewable search.
Uses the new crm-search-display directive to enable non-table subsearches.
No need for dynamically generated templates when we can just use the one generic `crm-search-display` element. Just requiring crmSearchDisplay, the dependencies sort themselves out and we don't need to declare `basePages` for every display.
a82658c to
229bf3e
Compare
| type: '@', | ||
| apiEntity: '@', | ||
| search: '<', | ||
| display: '<', | ||
| settings: '<', |
There was a problem hiding this comment.
@colemanw could we remove type and settings here and fetch them in the component based on search and display?
There was a problem hiding this comment.
Ah, yea they're probably usually contained within those arrays.
There was a problem hiding this comment.
@ufundo actually no, they're not. search and display are strings not arrays. The only array passed in is the settings. So all this stuff needs to be passed thru.
There was a problem hiding this comment.
So my thinking is they are logically dependent, rather than literally one includes the other.
My first though was something like:
this.$onInit = () => {
this.display = crmApi4('SearchDisplay', 'get', {
select: ['type', 'settings'],
where: [['name', '=', this.display], ['saved_search_id.name', '=', this.search]]
})[0];
...
It's an extra api request, and it wont work well in the editor, but it is much more convenient if you are writing html/smarty templates to not have to fetch those things in advance.
Maybe it could be a fallback/lazy option:
this.$onInit = () => {
if (!this.type || !this.settings) {
const savedDisplay = crmApi4('SearchDisplay', 'get', {
select: ['type', 'settings'],
where: [['name', '=', this.display], ['saved_search_id.name', '=', this.search]]
})[0];
this.type = this.type ? this.type : savedDisplay['type'];
this.settings = this.settings ? this.settings : savedDisplay['type'];
...
That would be more of a follow-up than a blocker.
There was a problem hiding this comment.
@ufundo it sounds like a good idea, but I think there's 2 potential problems:
- In a Subsearch scenario, with 50 rows per page (default) that's 50 extra ajax requests per subsearch!
- Users might not be allowed to call the
'SearchDisplay', 'get'ajax api depending on their permission levels.
If we really wanted to solve both problems, the metadata about the search display could be returned as part of the first searchDisplay.run request as an extra SearchDisplayRunResult property... In theory, I haven't tried it, you could fetch metadata about the search AND run the search in the same ajax request.
But that's a potentially YAGNI project and we've got bigger priorities IMO.
The real downside to this PR is that it loads ALL searchDisplay code ALL the time (e.g. all of ChartKit, etc). But js is browser-cacheable so probably not worth wringing our hands about.
0746483 to
e714f40
Compare
|
I tried subsearches with list displays and it works well. It's a big improvement UI and performance wise if all you care about is a simple summary. Which our clients want quite often. |
Overview
Enables Subsearches on any search display of any other type of search display.
Technical Details
I gave up on lazy-loading search display modules. So this adds a new
crm-search-displaywrapper directive which pre-loads all viewable search displays as dependencies.